🐛 Set Machine's BootstrapReady when there is no ConfigRef #11459
Conversation
k8s-ci-robot
added
area/machine
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @zaneb! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @zaneb. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| } | ||
| s.bootstrapConfig = obj | ||
|
|
||
| // If the bootstrap data is populated, set ready and return. | ||
| if m.Spec.Bootstrap.DataSecretName != nil { |
There was a problem hiding this comment.
Why does this come after reconciling the bootstrap config? The data secret name is either set by the user, in which case bootstrap config is not needed, or, it is set by the bootstrap provider, in which case the bootstrap config has already been reconciled. Wondering if we just need to move this block above?
There was a problem hiding this comment.
I believe #7394 has the answer to that:
to ensure the ownerReference, and other changes, are set before the dataSecretName is checked. This is designed to help in backup and restore use cases, or any other case where the ownerReference is removed.
| // Tolerate bootstrap object not found when the machine is being deleted. | ||
| // TODO: we can also relax this and tolerate the absence of the bootstrap ref way before, e.g. after node ref is set | ||
| return ctrl.Result{}, nil | ||
| if m.Spec.Bootstrap.ConfigRef != nil { |
There was a problem hiding this comment.
What about
| if m.Spec.Bootstrap.ConfigRef != nil { | |
| // If the bootstrap data secret is set by the user, set ready and return. | |
| if m.Spec.Bootstrap.ConfigRef == nil { | |
| if m.Spec.Bootstrap.DataSecretName == nil { | |
| return ctrl.Result{}, errors.New("either spec.bootstrap.dataSecretName or spec.bootstrap.configRef must be populated") | |
| } | |
| m.Status.BootstrapReady = true | |
| conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition) | |
| return ctrl.Result{}, nil | |
| } | |
| // Call generic external reconciler if we have an external reference. | |
| ... | |
| // Drop >> If the bootstrap data is populated, set ready and return. | |
| ... |
So we take care of user provided data secret first, and then we handle when data secret is generated by the bootstrap provider (without mixing the two use cases)
There was a problem hiding this comment.
That looks like it would work (though people thought the same about this). I guess it comes down to whether it's worse to duplicate the code that marks the bootstrap ready, or to duplicate an if statement.
There was a problem hiding this comment.
Feel free to add more tests to ensure the solution works and we don't have regressions (and that the fact that test did not catch the issue with the previous PR, is a clear signal that we need more tests).
WRT to the implementation options, I would prefer to mix up the two use cases in the code because the resulting code is simpler to read and to reason about and this usually pays back on the long run.
zaneb
force-pushed
the
machine-bootstrap-ready
branch
from
8ebd235
to
834bead
Compare
k8s-ci-robot
added
size/S
If there is no ConfigRef but the bootstrap data secret is set by the user (instead of a bootstrap provider), then BootstrapReady should be true. This is the case for MachineSet, and was originally the case for Machine since 5113f80. However, in d93eadc this changed as a side effect of ensuring that bootstrap config object can continue to be reconciled after the bootstrap provider has produced the bootstrap data secret. This change ensures that, once a bootstrap data secret exists, in the case of a ConfigRef it can still be reconciled, while in the case there is no ConfigRef, BootstrapReady is set. Signed-off-by: Zane Bitter <[email protected]>
zaneb
force-pushed
the
machine-bootstrap-ready
branch
from
834bead
to
f74c2e3
Compare
If there is no ConfigRef but the bootstrap data secret is set by the user (instead of a bootstrap provider), then BootstrapReady should be true. This is the case for MachineSet, and was originally the case for Machine since 5113f80. However, in d93eadc this changed as a side effect of ensuring that bootstrap config object can continue to be reconciled after the bootstrap provider has produced the bootstrap data secret.
This change ensures that, once a bootstrap data secret exists, in the case of a ConfigRef it can still be reconciled, while in the case there is no ConfigRef, BootstrapReady is set.
/area machine